Add a bunch of stuffs to Index.Remove() #398

yorah · 2013-04-18T16:25:59Z

With this PR, it is now possible to:

Pass ExplicitPathsOptions to Index.Remove()
Remove whole folders from the index
Remove files/folders from the index without removing them from the workdir (git rm --cached)
Clear conflicts state when removing entries from the index

This fixes #270 and #327.

Please note that the behavior is now consistent with Index.Stage()/Unstage():

passing pathspecs which don't match any files does not throw anymore.
passing explicit paths (by passing a non-null ExplicitPathsOptions) which don't match any files will throw.

This is a breaking change.

ethomson · 2013-04-18T16:34:48Z

I'm strongly in favor of changing the remove semantics to allow for rm --cached.

In doing this, we should probably start calling git_index_remove_bypath() so that conflicts are handled properly.

nulltoken · 2013-04-18T17:25:57Z

LibGit2Sharp.Tests/IndexFixture.cs

@@ -194,6 +194,50 @@ private static void InvalidMoveUseCases(string sourcePath, FileStatus sourceStat
            }
        }

+        public void CanRemoveAFolderThroughUsageOfPathspecs()


Aren't we missing a [Fact] attribute here?

As a matter of fact, yes we do!

nulltoken · 2013-04-18T17:27:51Z

@yishaigalatzer @anaisamp This PR should fix some of your requests.

nulltoken · 2013-04-18T17:51:27Z

LibGit2Sharp.Tests/IndexFixture.cs

+        }
+
+        [Fact]
+        public void RemovingAInvalidFileThrows()


As we now have only one case, could you please update the name of the test to actually express what kind of state is being invalid?

As we now have only one case

Or maybe are we missing some of them. What about FileStatus.TypeChanged for instance?

anaisamp · 2013-04-19T09:09:47Z

Thank you so much! Good job, you guys are awesome! :)

yorah · 2013-04-19T15:17:40Z

Here is an update:

Added lots of tests to cover git rm --cached
Tweaked Index.Remove() to allow for git rm --cached
Used git_index_remove_bypath in internal implementation

Missing:

Handling of git rm -f
Handling of TypeChanged records. There seems to be deeper things missing to allow for it at the moment (RetrieveStatus should be able to report it for example)
Tests to show what git_index_remove_bypath allows us to do now (@ethomson do you have ideas for that by any chance?)

@nulltoken would it be possible to address the first 2 points in another PR?

nulltoken · 2013-04-20T10:40:46Z

@nulltoken would it be possible to address the first 2 points in another PR?

Cool with me 👍
Will you work on them once this one is merged or should we create an issue to track those tasks?

Used git_index_remove_bypath in internal implementation

@yishaigalatzer I believe this should fix #325 as well.

yorah · 2013-04-20T20:24:59Z

Will you work on them once this one is merged or should we create an issue to track those tasks?

I will work on the git rm -f once this one is merged. For the TypeChanged one, I will open an issue to get some feedback first (about the best way tp add it). I have the feeling it may be more complex than it seems.

yorah · 2013-04-22T09:54:27Z

In the Index.Remove(), I throw a bunch of LibGit2SharpException. Would it be better to throw a strongly-typed exception, and if yes, how would you call it?

/cc @dahlbyk @haacked @carlosmn

dahlbyk · 2013-04-22T14:06:42Z

LibGit2Sharp/Index.cs

            }

-            if (dic.Count == 0)
+            foreach (string path in pathsList.Where(p => Directory.Exists(Path.Combine(wd, p))))


Possibly a premature optimization, but checking Directory.Exists() twice for every entry in pathsList seems like it's worth avoiding.

Thanks, fixed!

dahlbyk · 2013-04-22T14:18:50Z

In the Index.Remove(), I throw a bunch of LibGit2SharpException. Would it be better to throw a strongly-typed exception, and if yes, how would you call it?

👍

RemoveFromIndexException?

Note: it means that the paths passed to Remove() are pathspecs, and thus behavior has been aligned with Stage()/Unstage() => calling Remove() by passing a pathspec which doesn't match any paths doesn't throw anymore (breaking change).

This makes the behavior fully consistent with Index.Stage()/Unstage().

Fixes #327

Fixes #270

Lots of test cases added, to mimic git rm --cached behavior.

…bypath()

Beware, this is supported only for files which exist in the workdir (cf. comment in code). Partially fixes #325

yorah · 2013-04-23T09:07:38Z

RemoveFromIndexException?

❤️

yorah · 2013-04-23T09:46:38Z

Ok, time to look back at what we do in this PR.

There is one corner-case with the current approach: as we rely on Diff (for pathspecs expansion at least), and as libgit2's git_iterator_for_index() seem to only carea bout entries in stage level 0, it means that for merge conflicts (with stage level = 1, 2 or 3), we don't detect index entries if there is no corresponding file in the workdir. This limit is expressed in the last commit (@39f8947)

Note: with a RetrieveStatus approach, other problems would arise (we would need pathspecs parsing in lg2s, the ability to easily retrieve IndexEntries with StageLevel != 0, and some way to get meaningful merge statuses), linked to already existing issues: #279, #271

Another option would be to consider that this whole logic should be part of libgit2, but I'm afraid this might be considered a bit too high-level. @carlosmn Thoughts?

All in all, I think I would advocate merging this PR as is, and build from there. I think the tests are quite clear and express clearly the expectations, so should we refactor we have our bases covered.

ethomson · 2013-04-23T13:21:34Z

Does this mean that I can't call Remove() on a file that doesn't exist in the workdir but does have high-stage index entries?

I'm not sure that's such a corner case, actually; I added git_index_remove_bypath in the first place so that we could clear conflict entries when we wanted to "take" the delete side of an edit/delete conflict.

So having this would be very beneficial for us - it would be one less hack to drag forward.

I think that a flag to the index iterator to include high-stage entries may not be inappropriate if that would be helpful?

yorah · 2013-04-23T15:26:46Z

Does this mean that I can't call Remove() on a file that doesn't exist in the workdir but does have high-stage index entries?

Yes 😞

I'm not sure that's such a corner case, actually; I added git_index_remove_bypath in the first place so that we could clear conflict entries when we wanted to "take" the delete side of an edit/delete conflict.

Well, at the moment you can still do it, just not when the file is missing from the workdir.

I think that a flag to the index iterator to include high-stage entries may not be inappropriate if that would be helpful?

I'm afraid this won't be enough. From what I understand so far, the semantics returned by git diff when showing a merge is not the same as the "standard" format (look at the -cc option). Maybe someone from libgit2 could jump in at this point?

Edit (proposal): what about letting the repo.Index namespace handle only the stage level 0 (ie: it means going back to calling git_index_remove(), and add a repo.Conflicts.RemoveFile(string relativePath) which would call git_index_remove_bypath() ? The idea I'm trying to play with is to move all the things related to stage level 1, 2 and 3 in the repo.Conflicts namespace.

ethomson · 2013-04-23T16:00:37Z

I'm afraid this won't be enough. From what I understand so far, the semantics returned by git diff when showing a merge is not the same as the "standard" format (look at the -cc option). Maybe someone from libgit2 could jump in at this point?

What can libgit2 do differently to enhance this scenario?

Edit (proposal): what about letting the repo.Index namespace handle only the stage level 0 (ie: it means going back to calling git_index_remove(), and add a repo.Conflicts.RemoveFile(string relativePath) which would call git_index_remove_bypath() ? The idea I'm trying to play with is to move all the things related to stage level 1, 2 and 3 in the repo.Conflicts namespace.

I don't think you should go back to git_index_remove here; remove_bypath is a very simple and very safe wrapper around remove that moves conflicts to the REUC. Unless you can think of an explicit case where you do not want to move conflicts to the REUC but aren't working on index entries at a lower level, then it should probably be preferred to avoid confusion.

I'm okay with an explicit Conflicts.Remove(...) that calls git_index_remove_bypath -- this is basically what we're doing now -- but I was really hopeful that we could have a single call to remove index entries instead of having to know whether the file was on-disk or not, etc, etc.

yorah · 2013-04-24T13:11:44Z

What can libgit2 do differently to enhance this scenario?

It would be nice to have libgit2 able to report conflicts in git_status_foreach_ext(). I think this discussion has already been initiated/envisioned by @arrbee in #271.

nulltoken · 2013-04-24T13:21:25Z

Unless someone strongly opposes, I'm going to merge this as is later today.

@yorah Could you please create an issue so we that we track this?

/* Conflicts clearing through Index.Remove() only works when a version of the entry exists in the workdir.
 * This is because libgit2's git_iterator_for_index() seem to only care about stage level 0.
 * Corrolary: other cases only work out of sheer luck (however, the behaviour is stable, so I guess we
 *   can rely on it for the moment.
 * [InlineData(true, "ancestor-only.txt", false, false, FileStatus.Nonexistent, 0)]
 * [InlineData(false, "ancestor-only.txt", false, false, FileStatus.Nonexistent, 0)]
 */

yorah · 2013-04-24T13:59:49Z

@yorah Could you please create an issue so we that we track this?

#411

nulltoken · 2013-04-24T14:50:07Z

@yorah Could you please create an issue so we that we track this?

#411

🎱

nulltoken · 2013-04-24T18:13:09Z

AMAZING work!

Merged.

nulltoken reviewed Apr 18, 2013
View reviewed changes

dahlbyk reviewed Apr 22, 2013
View reviewed changes

yorah added 11 commits April 23, 2013 11:00

Add RemoveFromIndexException

2a2ad53

Make Index.Remove() diff-based

b57055e

Note: it means that the paths passed to Remove() are pathspecs, and thus behavior has been aligned with Stage()/Unstage() => calling Remove() by passing a pathspec which doesn't match any paths doesn't throw anymore (breaking change).

Allow passing ExplicitPathsOptions to Index.Remove()

60e5f66

This makes the behavior fully consistent with Index.Stage()/Unstage().

Make Index.Remove() able to remove folders

00e40d8

Fixes #327

Make Index.Remove() able to remove only from index

0d4bc09

Fixes #270

Improve support for git rm --cached semantics

ec9b617

Lots of test cases added, to mimic git rm --cached behavior.

Move Index.Remove() tests to a separate file

b7decea

Switch internal implementation of Index.Remove() to git_index_remove_…

a95144c

…bypath()

Cleanup ConflictFixture

0efb0fc

Add BaseFixture.CloneMergedTestRepo()

f8d94ae

Add test to ensure conflicts are cleared when calling Index.Remove()

39f8947

Beware, this is supported only for files which exist in the workdir (cf. comment in code). Partially fixes #325

yorah mentioned this pull request Apr 24, 2013

Improve behaviour of Index.Remove() #411

Closed

3 tasks

nulltoken closed this Apr 24, 2013

ethomson mentioned this pull request Jun 18, 2014

Remove conflicts in Index.Remove #768

Merged

Add a bunch of stuffs to Index.Remove() #398

Add a bunch of stuffs to Index.Remove() #398

Uh oh!

Conversation

yorah commented Apr 18, 2013

Uh oh!

ethomson commented Apr 18, 2013

Uh oh!

nulltoken Apr 18, 2013

Choose a reason for hiding this comment

Uh oh!

yorah Apr 19, 2013

Choose a reason for hiding this comment

Uh oh!

nulltoken commented Apr 18, 2013

Uh oh!

nulltoken Apr 18, 2013

Choose a reason for hiding this comment

Uh oh!

nulltoken Apr 18, 2013

Choose a reason for hiding this comment

Uh oh!

anaisamp commented Apr 19, 2013

Uh oh!

yorah commented Apr 19, 2013

Uh oh!

nulltoken commented Apr 20, 2013

Uh oh!

yorah commented Apr 20, 2013

Uh oh!

yorah commented Apr 22, 2013

Uh oh!

dahlbyk Apr 22, 2013

Choose a reason for hiding this comment

Uh oh!

yorah Apr 23, 2013

Choose a reason for hiding this comment

Uh oh!

dahlbyk commented Apr 22, 2013

Uh oh!

yorah commented Apr 23, 2013

Uh oh!

yorah commented Apr 23, 2013

Uh oh!

ethomson commented Apr 23, 2013

Uh oh!

yorah commented Apr 23, 2013

Uh oh!

ethomson commented Apr 23, 2013

Uh oh!

yorah commented Apr 24, 2013

Uh oh!

nulltoken commented Apr 24, 2013

Uh oh!

yorah commented Apr 24, 2013

Uh oh!

nulltoken commented Apr 24, 2013

Uh oh!

nulltoken commented Apr 24, 2013

Uh oh!

Uh oh!